Add essential API endpoints and validation#33
Conversation
Implement missing CRUD operations and validation to complete core functionality:
- Add health check endpoint (GET /health) for monitoring database connectivity
- Complete Opening CRUD operations:
* GET /openings/{id} - retrieve single opening with faculty info
* PUT /openings/{id} - update opening (with ownership validation)
* DELETE /openings/{id} - delete opening and related notifications
- Add OpeningUpdate model for partial updates
- Add application withdrawal (DELETE /applications/withdraw/{id})
* Only allows withdrawing pending applications
- Add mark all notifications read (PUT /notifications/read-all)
- Add deadline validation to application submission
* Block applications past deadline
* Block applications to closed openings
* Handle both date and datetime types from Neo4j
All update/delete operations include ownership validation to ensure only
the resource owner can modify their data.
📝 WalkthroughWalkthroughThis PR adds documentation (GitHub PR template and expanded README), enhances the API initialization with a health check endpoint and static file serving, introduces an Changes
Sequence DiagramssequenceDiagram
actor Student
participant API
participant Neo4j
Note over Student,Neo4j: Apply to Opening
Student->>API: POST /apply/{opening_id}
API->>Neo4j: Fetch opening (check deadline & status)
Neo4j-->>API: Opening data
alt Opening closed or deadline passed
API-->>Student: ❌ 400 Bad Request
else Validation passes
API->>Neo4j: Check if already applied (APPLIED_TO)
Neo4j-->>API: Relationship exists?
alt Already applied
API-->>Student: ❌ 409 Conflict
else First application
API->>Neo4j: Create APPLIED_TO relationship
API->>Neo4j: Create Notification node
Neo4j-->>API: Success
API-->>Student: ✓ 201 Application created
end
end
Note over Student,Neo4j: Withdraw Application
Student->>API: DELETE /withdraw/{opening_id}
API->>Neo4j: Validate APPLIED_TO exists & pending status
Neo4j-->>API: Status check
alt Status not pending
API-->>Student: ❌ 400 Invalid status
else Valid pending status
API->>Neo4j: Delete APPLIED_TO relationship
Neo4j-->>API: Success
API-->>Student: ✓ 200 Withdrawn
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
app/models/openings.py (1)
18-27: Consider validating thestatusfield.The
statusfield accepts any string, but per the comment it should only be"Active"or"Closed". UsingLiteralwould catch invalid values at the API boundary.🔎 Proposed fix
from pydantic import BaseModel -from typing import List, Optional +from typing import List, Literal, Optional from datetime import date class OpeningUpdate(BaseModel): """All fields optional for partial updates.""" title: Optional[str] = None description: Optional[str] = None required_skills: Optional[List[str]] = None expected_duration: Optional[str] = None target_years: Optional[List[str]] = None min_cgpa: Optional[float] = None deadline: Optional[date] = None - status: Optional[str] = None # "Active" or "Closed" + status: Optional[Literal["Active", "Closed"]] = NoneREADME.md (2)
105-112: Add language specifier to fenced code block.Per linting, this code block should have a language specifier. Since it's pseudo-code/formula, use
textorplaintext.🔎 Proposed fix
- ``` + ```text Base Score = (Matched Skills / Total Required) × 100
434-471: Add language specifier to project structure block.The directory tree should have a language specifier for consistent linting.
🔎 Proposed fix
-``` +```text gurusetu-backend/ ├── app/
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/pull_request_template.mdREADME.mdapp/main.pyapp/models/openings.pyapp/routers/applications.pyapp/routers/notifications.pyapp/routers/openings.py
🧰 Additional context used
🧬 Code graph analysis (4)
app/routers/openings.py (3)
app/models/openings.py (2)
OpeningCreate(6-15)OpeningUpdate(18-27)app/core/security.py (1)
get_current_user(58-77)app/core/database.py (2)
get_session(33-42)close(28-31)
app/routers/applications.py (2)
app/core/security.py (1)
get_current_user(58-77)app/core/database.py (2)
get_session(33-42)close(28-31)
app/main.py (1)
app/core/database.py (2)
get_session(33-42)close(28-31)
app/routers/notifications.py (2)
app/core/database.py (2)
close(28-31)get_session(33-42)app/core/security.py (1)
get_current_user(58-77)
🪛 markdownlint-cli2 (0.18.1)
README.md
105-105: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
434-434: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.10)
app/routers/openings.py
37-37: Abstract raise to an inner function
(TRY301)
42-42: Do not catch blind exception: Exception
(BLE001)
43-43: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
44-44: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
53-53: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
68-69: Abstract raise to an inner function
(TRY301)
74-74: Abstract raise to an inner function
(TRY301)
103-103: Consider moving this statement to an else block
(TRY300)
106-106: Do not catch blind exception: Exception
(BLE001)
107-107: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
108-108: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
116-116: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
137-138: Abstract raise to an inner function
(TRY301)
140-140: Consider moving this statement to an else block
(TRY300)
143-143: Do not catch blind exception: Exception
(BLE001)
144-144: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
145-145: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
153-153: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
app/routers/applications.py
22-22: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
40-41: Abstract raise to an inner function
(TRY301)
44-47: Abstract raise to an inner function
(TRY301)
61-61: Do not catch blind exception: Exception
(BLE001)
62-62: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
63-64: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
88-88: Abstract raise to an inner function
(TRY301)
92-93: Abstract raise to an inner function
(TRY301)
104-105: Abstract raise to an inner function
(TRY301)
114-115: Abstract raise to an inner function
(TRY301)
151-151: Consider moving this statement to an else block
(TRY300)
153-153: Do not catch blind exception: Exception
(BLE001)
155-156: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
212-212: Consider moving this statement to an else block
(TRY300)
214-214: Do not catch blind exception: Exception
(BLE001)
216-216: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
app/main.py
67-67: Consider moving this statement to an else block
(TRY300)
68-68: Do not catch blind exception: Exception
(BLE001)
app/routers/notifications.py
51-51: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (5)
.github/pull_request_template.md (1)
1-13: LGTM!Clean and minimal PR template with essential sections for description, changes, and related issues.
app/routers/notifications.py (1)
50-65: LGTM!The
mark_all_notifications_readendpoint correctly filters by user ownership, only updates unread notifications, and properly manages the database session.app/routers/openings.py (1)
12-46: Verify that public access to openings is intended.The
get_openingendpoint has no authentication requirement, meaning anyone can view opening details including faculty information. If this is intentional for public browsing, this is fine. Otherwise, consider addingDepends(get_current_user).app/routers/applications.py (2)
21-66: LGTM!The
withdraw_applicationendpoint correctly validates student role, checks pending status before allowing withdrawal, and properly re-raisesHTTPExceptionbefore catching generic exceptions.
96-105: Good handling of Neo4j date types.The deadline validation correctly handles both Neo4j native date types (via
to_native()) and Python datetime/date objects. This prevents type-related runtime errors when comparing dates.
| @app.get("/health") | ||
| def health_check(): | ||
| """Health check endpoint for monitoring and load balancers.""" | ||
| session = None | ||
| try: | ||
| session = db.get_session() | ||
| session.run("RETURN 1") | ||
| return {"status": "healthy", "database": "connected"} | ||
| except Exception as e: | ||
| return {"status": "unhealthy", "database": "disconnected", "error": str(e)} | ||
| finally: | ||
| if session: | ||
| session.close() |
There was a problem hiding this comment.
Health check should return HTTP 503 when unhealthy.
Load balancers and monitoring tools typically rely on HTTP status codes, not response body content. Returning HTTP 200 with "status": "unhealthy" will cause the service to appear healthy even when the database is down.
🔎 Proposed fix
@app.get("/health")
def health_check():
"""Health check endpoint for monitoring and load balancers."""
session = None
try:
session = db.get_session()
session.run("RETURN 1")
return {"status": "healthy", "database": "connected"}
except Exception as e:
- return {"status": "unhealthy", "database": "disconnected", "error": str(e)}
+ raise HTTPException(
+ status_code=503,
+ detail={"status": "unhealthy", "database": "disconnected", "error": str(e)}
+ )
finally:
if session:
session.close()Note: You'll need to import HTTPException from fastapi at the top of the file.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
67-67: Consider moving this statement to an else block
(TRY300)
68-68: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In app/main.py around lines 60 to 72, the health_check endpoint currently
returns an HTTP 200 body even when the database is down; change it to raise
fastapi.HTTPException(status_code=503, detail=...) when the DB check fails so
clients/load‑balancers receive a 503 status; ensure you import HTTPException
from fastapi at the top, keep the successful path returning a 200 response, and
include the error message in the exception detail (and still close the session
in finally).
| except Exception as e: | ||
| print(f"Application Error: {e}") | ||
| raise HTTPException(status_code=500, detail="Server error processing application") | ||
| raise HTTPException( | ||
| status_code=500, detail="Server error processing application") |
There was a problem hiding this comment.
HTTPException is swallowed and replaced with a generic 500 error.
The except Exception block catches HTTPException raised earlier (lines 88, 92-93, 104-105, 114-115), losing the specific error details. User-facing validation errors (400, 404) will incorrectly return as 500 errors.
🔎 Proposed fix
return {"message": "Application submitted successfully"}
+ except HTTPException:
+ raise
except Exception as e:
print(f"Application Error: {e}")
raise HTTPException(
status_code=500, detail="Server error processing application")🧰 Tools
🪛 Ruff (0.14.10)
153-153: Do not catch blind exception: Exception
(BLE001)
155-156: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In app/routers/applications.py around lines 153 to 156, the broad except
Exception catches and converts previously raised HTTPException instances into a
generic 500, losing original status and detail; change the handler to re-raise
HTTPException unchanged (e.g., check isinstance(e, HTTPException) and raise) or
add a separate except HTTPException: raise block above the generic except, and
only handle/log non-HTTP exceptions before raising a new 500 with a concise
message.
| except Exception as e: | ||
| print(f"Status Update Error: {e}") | ||
| raise HTTPException(status_code=500, detail="Failed to update status") |
There was a problem hiding this comment.
Same issue: HTTPException is swallowed in update_application_status.
The generic except Exception catches the HTTPException from line 167, causing the "Access denied" 403 error to become a 500 error.
🔎 Proposed fix
return {"message": f"Applicant marked as {data.status}"}
+ except HTTPException:
+ raise
except Exception as e:
print(f"Status Update Error: {e}")
raise HTTPException(status_code=500, detail="Failed to update status")🧰 Tools
🪛 Ruff (0.14.10)
214-214: Do not catch blind exception: Exception
(BLE001)
216-216: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In app/routers/applications.py around lines 214-216, the broad "except Exception
as e" block is catching and converting an already-raised HTTPException (e.g.,
the 403 "Access denied") into a 500; modify the error handling so HTTPException
instances are not swallowed: first check for and re-raise HTTPException (or add
an except HTTPException: raise) and then handle other exceptions (log the error)
and raise a new HTTPException(status_code=500, detail="Failed to update status")
for unexpected errors. Ensure you remove or replace the print with proper
logging if present.
| delete_query = """ | ||
| MATCH (f:User {user_id: $faculty_id})-[:POSTED]->(o:Opening {id: $opening_id}) | ||
| OPTIONAL MATCH (o)-[r]-() // All relationships | ||
| OPTIONAL MATCH (n:Notification {trigger_id: $opening_id}) | ||
| DETACH DELETE o, n | ||
| RETURN count(o) AS deleted | ||
| """ | ||
| result = session.run( | ||
| delete_query, faculty_id=current_user["user_id"], opening_id=opening_id).single() | ||
|
|
||
| if not result or result["deleted"] == 0: | ||
| raise HTTPException( | ||
| status_code=404, detail="Opening not found or you don't have permission") |
There was a problem hiding this comment.
count(o) after DETACH DELETE may not work as expected.
After DETACH DELETE o, the variable o is unbound, so count(o) may return 0 or cause issues depending on Neo4j version. Consider capturing a marker before deletion.
🔎 Proposed fix
delete_query = """
MATCH (f:User {user_id: $faculty_id})-[:POSTED]->(o:Opening {id: $opening_id})
+ WITH o, o.id AS matched_id
OPTIONAL MATCH (o)-[r]-() // All relationships
OPTIONAL MATCH (n:Notification {trigger_id: $opening_id})
DETACH DELETE o, n
- RETURN count(o) AS deleted
+ RETURN matched_id IS NOT NULL AS deleted
"""
result = session.run(
delete_query, faculty_id=current_user["user_id"], opening_id=opening_id).single()
- if not result or result["deleted"] == 0:
+ if not result or not result["deleted"]:
raise HTTPException(
status_code=404, detail="Opening not found or you don't have permission")🧰 Tools
🪛 Ruff (0.14.10)
137-138: Abstract raise to an inner function
(TRY301)
🤖 Prompt for AI Agents
In app/routers/openings.py around lines 126 to 138, the Cypher uses count(o)
after DETACH DELETE which is unsafe because o is unbound after deletion; change
the query to capture a marker/count before deleting (for example use WITH to
compute/count o or collect a marker into a variable, then DETACH DELETE using
that variable, and finally RETURN the previously computed count), and keep the
Python call signature the same so the result["deleted"] reflects the number
deleted.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.